Optimize `Date::to_calendar` #7089

robertbastian · 2025-10-14T13:00:04Z

Currently non-Gregorian calendars implement to_calendar as Date<A> -> RataDie -> Date<Iso> -> RataDie -> Date. This PR cuts out the ISO conversions to make it Date<A> -> RataDie -> Date.

Gregorian calendars currently use Date<A> -> Date<Iso> -> Date, where both of these conversions are free because internally they are the same. This PR preserves this behaviour by adding a HAS_CHEAP_ISO_CONVERSION const to the Calendar trait, and if both A and B have this, it goes through ISO instead of RD.

AnyCalendar still uses the Date<A> -> RataDie -> Date<Iso> -> RataDie -> Date path, if we wanted to optimise this we'd need HAS_CHEAP_ISO_CONVERSION to be a method instead of a const.

sffc · 2025-10-20T16:59:41Z

/gemini review

gemini-code-assist

Code Review

This pull request optimizes date conversions between different calendar systems by introducing a more direct conversion path via RataDie for non-Gregorian calendars, while preserving the efficient ISO-based conversion for Gregorian ones. This is achieved by adding a HAS_CHEAP_ISO_CONVERSION constant to the Calendar trait. The changes are well-structured and result in cleaner, more efficient code, especially in the Indian calendar implementation. I've found one potential issue with handling overflow in the new Indian calendar conversion logic, which I've detailed in a comment. Overall, this is a great improvement.

components/calendar/src/cal/indian.rs

sffc

I'm fairly neutral on whether this is an improvement or not. It deletes more lines of code than it adds so I'll approve based on that.

sffc · 2025-10-20T17:29:54Z

components/calendar/src/date.rs

+        let inner =
+            if A::Calendar::HAS_CHEAP_ISO_CONVERSION && A2::Calendar::HAS_CHEAP_ISO_CONVERSION {
+                let iso = self.calendar.as_calendar().to_iso(self.inner());
+                calendar.as_calendar().from_iso(iso)
+            } else {
+                let rd = self.calendar.as_calendar().to_rata_die(self.inner());
+                calendar.as_calendar().from_rata_die(rd)
+            };


Suggestion: I wonder if this PR would be better going through an intermediate such as

enum RataDieOrIso { RataDie(RataDie), Iso(Date<Iso>) }

Calendars would return Iso if cheap or RataDie otherwise, and constructors would have code such as

// Calendar with cheap ISO conversion let iso_date = match RataDieOrIso { RataDieOrIso::RataDie(rata_die) => Iso.from_rata_die(rata_die), RataDieOrIso::Iso(iso_date) => iso_date, }; // Calendar with expensive ISO conversion let rata_die = match RataDieOrIso { RataDieOrIso::RataDie(rata_die) => rata_die, RataDieOrIso::Iso(iso_date) => Iso.to_rata_die(iso_date), };

The problem is that ISO is only cheaper if it's cheap for both calendars, and your proposal only considers the source calendar.

I think it would work (consider Hebrew as expensive ISO and ROC as cheap ISO):

Hebrew to Hebrew: converts through RataDie. OK.

ROC to ROC: converts through ISO. OK, all cheap.

Hebrew to ROC: Hebrew returns RataDie (expensive), then ROC converts it to ISO (casioneri), then ISO to ROC (cheap).

ROC to Hebrew: ROC returns ISO (cheap), then Hebrew converts the ISO to RataDie (casioneri) and RataDie to Hebrew (expensive).

Hmm I guess that works. However, accepting RataDieOrIso means the code path being taken is not statically known, so more code gets linked and optimisations might be inhibited

I posted another thought in #7089 (comment) which I think is better than the one in this thread.

components/calendar/src/calendar.rs

robertbastian · 2025-10-20T17:45:23Z

I'm fairly neutral on whether this is an improvement or not. It deletes more lines of code than it adds so I'll approve based on that.

You have absolutely no opinion on the work this avoids for conversions between non-Gregorian calendars?

sffc · 2025-10-20T18:01:07Z

I'm fairly neutral on whether this is an improvement or not. It deletes more lines of code than it adds so I'll approve based on that.

You have absolutely no opinion on the work this avoids for conversions between non-Gregorian calendars?

Well the narrower fix to that bug would be to make to_calendar unconditionally use RataDie, or to land the casioneri code so that rata die to ISO is very cheap. I haven't formed an opinion on whether the bigger refactor is worthwhile for implementing the change.

robertbastian · 2025-10-20T18:09:02Z

Well the narrower fix to that bug would be to make to_calendar unconditionally use RataDie

I'm happy to do that, I thought this would be the narrower fix

sffc

I'm fine with this landing but let's get a third opinion from @Manishearth on the approach

sffc · 2025-10-20T19:21:49Z

components/calendar/src/calendar.rs

+    /// Whether `from_iso`/`to_iso` is more efficient
+    /// than `from_rata_die`/`to_rata_die`.
+    const HAS_CHEAP_ISO_CONVERSION: bool;


Suggestion: Another design here would be

fn cheap_to_iso(&self, date: &Self::DateInner) -> Option<IsoDateInner>;

from_iso would retain the default impl as you have here. Then the Date code would be

match from_date.cheap_to_iso(inner) { Some(iso_date) => to_calendar.from_iso(iso_date), None => to_calendar.from_rata_die(from_date.to_rata_die()) }

All of the cheap_to_iso impls should be marked as #[inline] so that DCE works nicely and avoids the branch.

I think all the pairs work:

ROC to ROC: uses cheap_to_iso and from_iso. OK

Hebrew to Hebrew: uses to_rata_die and from_rata_die. OK

ROC to Hebrew: uses cheap_to_iso and from_iso, where from_iso internally uses the default impl to_rata_die and from_rata_die. OK

Hebrew to ROC: uses to_rata_die and from_rata_die. OK

AnyCalendar still uses the Date<A> -> RataDie -> Date<Iso> -> RataDie -> Date path, if we wanted to optimise this we'd need HAS_CHEAP_ISO_CONVERSION to be a method instead of a const.

I think the solution here is basically this.

Manishearth

I'm generally not super worried about RD-ISO conversions, but sure, why not.

utils/calendrical_calculations/src/helpers.rs

sffc

I missed the part about AnyCalendar not benefiting from this. I think we should do a method in order for that to work. We have methods on Calendar for everything else that could be const specifically because we want AnyCalendar to work.

sffc · 2025-10-21T15:09:13Z

components/calendar/src/cal/chinese.rs

    }

-    const HAS_CHEAP_ISO_CONVERSION: bool = false;
+    fn has_cheap_iso_conversion(&self) -> bool {


Suggestion: mark all the methods as #[inline]

it shouldn't be necessary for in-crate usages

Manishearth

I am not a huge fan of adding yet another Calendar API, but it's fine.

robertbastian force-pushed the convert branch 3 times, most recently from 528b4f9 to dd1948c Compare October 20, 2025 12:26

robertbastian added 2 commits October 20, 2025 16:49

iso

c666a29

rd is slightly cheaper for Indian

95a7211

robertbastian force-pushed the convert branch from dd1948c to 95a7211 Compare October 20, 2025 14:49

robertbastian marked this pull request as ready for review October 20, 2025 14:49

robertbastian requested review from Manishearth and sffc as code owners October 20, 2025 14:49

gemini-code-assist bot reviewed Oct 20, 2025

View reviewed changes

components/calendar/src/cal/indian.rs Outdated Show resolved Hide resolved

sffc previously approved these changes Oct 20, 2025

View reviewed changes

saturate

5e85b52

robertbastian dismissed sffc’s stale review via 5e85b52 October 20, 2025 17:37

robertbastian requested a review from a team as a code owner October 20, 2025 17:37

nit

9c08a52

robertbastian requested a review from sffc October 20, 2025 17:42

Merge remote-tracking branch 'upstream/main' into convert

1d4550d

sffc reviewed Oct 20, 2025

View reviewed changes

Manishearth previously approved these changes Oct 20, 2025

View reviewed changes

utils/calendrical_calculations/src/helpers.rs Show resolved Hide resolved

sffc requested changes Oct 20, 2025

View reviewed changes

fn

e5c99ae

robertbastian dismissed Manishearth’s stale review via e5c99ae October 21, 2025 12:09

robertbastian requested a review from sffc October 21, 2025 12:10

sffc approved these changes Oct 21, 2025

View reviewed changes

robertbastian merged commit bfc038a into unicode-org:main Oct 21, 2025
30 checks passed

robertbastian deleted the convert branch October 21, 2025 15:13

Manishearth reviewed Oct 21, 2025

View reviewed changes

Optimize Date::to_calendar #7089

Optimize Date::to_calendar #7089

Uh oh!

Conversation

robertbastian commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sffc commented Oct 20, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

sffc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

robertbastian commented Oct 20, 2025

Uh oh!

sffc commented Oct 20, 2025

Uh oh!

robertbastian commented Oct 20, 2025

Uh oh!

sffc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manishearth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sffc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Manishearth left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Optimize `Date::to_calendar` #7089

Optimize `Date::to_calendar` #7089

robertbastian commented Oct 14, 2025 •

edited

Loading